Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Luxon #345

Merged
merged 22 commits into from
Apr 11, 2018
Merged

Luxon #345

merged 22 commits into from
Apr 11, 2018

Conversation

Ponjimon
Copy link
Contributor

@Ponjimon Ponjimon commented Apr 10, 2018

  • I have changed my target branch to develop 👊

Issue #61

Description

As requested, here is the PR. I had to do some changes to package.json, I hope they are okay. All tests are still passing. Tell me if something needs to be changed.

@Ponjimon
Copy link
Contributor Author

> cross-env NODE_ICU_DATA=node_modules/full-icu jest && npm run typescript
node: could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters)

Hm, that's weird. It runs fine on my machine. I need to set this environment variable for the localization. Luxon is using native Intl

@dmtrKovalenko
Copy link
Member

@lookapanda Can you please refer build failing and change your target branch to develop

@Ponjimon
Copy link
Contributor Author

What do you mean by "refer build failing"?

@Ponjimon Ponjimon changed the base branch from master to develop April 10, 2018 14:13
@dmtrKovalenko
Copy link
Member

@lookapanda
2018-04-10 17 44 14

@Ponjimon
Copy link
Contributor Author

#345 (comment)

I was talking about the failing build. I don't know why the build fails, it runs locally.

@dmtrKovalenko
Copy link
Member

[email protected] all-tests /home/travis/build/dmtrKovalenko/material-ui-pickers/lib
cross-env NODE_ICU_DATA=node_modules/full-icu jest && npm run typescript
node: could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters)

@dmtrKovalenko
Copy link
Member

@lookapanda why does it need at all?

@Ponjimon
Copy link
Contributor Author

Because I wanted to also test that the localization is properly working.

@cherniavskii
Copy link
Member

I'm looking into this

@cherniavskii
Copy link
Member

ok, tests are passing now

@dmtrKovalenko
Copy link
Member

Good job 👍
Just change your target branch to develop

@Ponjimon
Copy link
Contributor Author

lookapanda wants to merge 22 commits into dmtrKovalenko:develop from lookapanda:luxon
Didn't I already do that?

@dmtrKovalenko
Copy link
Member

@lookapanda Oh, sorry, I was from phone and misslead that you backported master to your branch. Thanks for your work!

@dmtrKovalenko dmtrKovalenko merged commit b09fa89 into mui:develop Apr 11, 2018
@@ -44,13 +44,15 @@
},
"dependencies": {
"babel-runtime": "^6.26.0",
"luxon": "^1.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, shouldn't luxon be a dev dependency? And also be located in externalDependencies along with moment and date-fns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeez, yes, I totally forgot about that. I just ran yarn add luxon locally which adds that as a dependency automatically (like npm install --save. Yes, that should be changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries ;)
would you like to submit a PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be great to document luxon localization on our docs page

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that the performance with Luxon is not really that great compared to, for example, the DateFns util. I will investigate this later this week.

@Ponjimon
Copy link
Contributor Author

Okay, I investigated a bit about the performance. Any idea how we could improve the performance of getWeekArray?
I ran a benchmark:

Luxon: getWeekArray x 1,081 ops/sec ±4.26% (89 runs sampled)
DateFns: getWeekArray x 67,535 ops/sec ±1.31% (91 runs sampled)

As you can see, DateFns is much, much faster. There is nothing really one can do about that fact as date-fns utilizes JS dates directly and Luxon works with wrappers, but can we improve the performance somehow?

It's very much noticeable when you open the date picker, for example. It takes some time until the modal opens.

@cherniavskii
Copy link
Member

@lookapanda from a fast look, looks like getWeekArray makes too much iterations:

https://github.com/dmtrKovalenko/material-ui-pickers/blob/b09fa89a92f00a3f876e1c2ce6006ef3e05a41ad/lib/src/utils/luxon-utils.js#L127-L149

There are two maps and forEach, which doesn't make a lot of sense for me.

That being said, if poor performance is caused by Luxon itself - we have nothing to do with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants